Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failing tests for IDN TLD that contains uppercase #64

Conversation

rugabarbo
Copy link

Q A
QA yes

These are just failing tests for top-level IDN domains that contain uppercase letters.

I tried to fix it, but found that the fix would be non-trivial. The problem is caused by using the built-in PHP functions strtoupper() and strtolower(). For example:

if (! in_array(strtolower($this->tld), $this->validTlds)

These functions do not work correctly with multi-byte encodings. But for processing multi-byte encodings, the code uses wrappers in the \Laminas\Stdlib\StringWrapper namespace, which do not yet contain functions strtoupper() and strtolower().

Thus, a full fix will look like this:

  • Add to \Laminas\Stdlib\StringWrapper\StringWrapperInterface methods strtoupper() and strtolower()
  • Implement these in the concrete wrappers
  • Release new version of https://github.com/laminas/laminas-stdlib
  • Use the new methods to fix the problem in the current package

So this is a subject for discussion...
Maybe I misunderstood something.

@rugabarbo rugabarbo changed the title Failing tests for TLD that contains uppercase Failing tests for IDN TLD that contains uppercase May 18, 2020
@weierophinney
Copy link
Member

Alternately, we could depend on symfony/polyfill-iconv and/or symfony/polyfill-mbstring, allowing us to use the relevant iconv and/or mbstring functions directly in the code. On systems that have the extension(s) installed, these are ignored, but otherwise, we'd still be able to rely on them, without needing to write our own wrappers.

@rugabarbo
Copy link
Author

rugabarbo commented May 18, 2020

As I see it, a strategic decision is needed here, because problems with multi-byte strings will occur over and over again in a wide variety of components.

Possible options:

  1. Develop further your own string wrappers in the package https://github.com/laminas/laminas-stdlib without using third-party components.
  2. Develop further your own string wrappers in the package https://github.com/laminas/laminas-stdlib using Symfony components (under the hood).
  3. Declare your own string wrappers as deprecated components and switch from them to using Symfony components directly.

@ghost

This comment was marked as spam.

@ghost

This comment was marked as spam.

@rugabarbo

This comment was marked as resolved.

@glensc
Copy link

glensc commented Aug 11, 2020

@rugabarbo perhaps you assumed the polyfills that @weierophinney mentioned are Symfony components, they are not, they are absolutely framework agnostic, they are just composer packages under Symfony namespace:

but if you meant really components, you mean string component? then it would require to use Symfony 5.0 which in turn requires PHP 7.2:

but this project is yet at PHP 7.1:

I personally would just go using polyfills, which can be replaced with real extensions using composer "replace" key:

@geerteltink geerteltink added this to the 2.14.0 milestone Sep 12, 2020
@geerteltink geerteltink changed the base branch from master to 2.14.x September 12, 2020 07:45
@weierophinney weierophinney force-pushed the failing-tests-for-tld-that-contains-uppercase branch 2 times, most recently from 1031676 to 6342fd5 Compare January 6, 2021 22:48
@weierophinney
Copy link
Member

I've pushed changes that demonstrate an attempt at fixing this using symfony/polyfill-mbstring. I installed that package, and then updated all string-related functions that have mbstring equivalents to the mbstring versions. When I was done, I noted the following:

  • Your first new test case passed.
  • The second new test case failed.
  • The lower-case version of the second case, which already existed, also failed.

Out of curiousity, I tried the same experiment using symfony/polyfill-iconv. However, in that scenario, the tests you've created continued to fail entirely.

I'm not quite sure where to go with this at this point, but I've left my symfony/polfill-mbstring experiment in here for you to work with. You'll need to fetch and rebase your branch.

rugabarbo and others added 2 commits July 14, 2021 08:36
…dator

This patch adds symfony/polyfill-mbstring as a requirement, and updates the `Hostname` validator to use mbstring equivalents of various string operations, where they are available.

Locally, one of the two new tests passes, but one original test ("UTF-8 label + UTF-8 TLD (cyrillic)") now fails — which may be a problem with my own locale.

Signed-off-by: Matthew Weier O'Phinney <[email protected]>
@weierophinney weierophinney force-pushed the failing-tests-for-tld-that-contains-uppercase branch from 6342fd5 to ebaa162 Compare July 14, 2021 13:43
@weierophinney
Copy link
Member

I've rebased this off current 2.14.x sources so that it can pick up our new CI. However, the tests as presented are still failing and it looks like it's likely due to the letter casing. I attempted to change the regexp to add the u flag, but that made no difference, so I did not push that change here.

At this point, I need somebody with experience in i18n casing issues to assist.

@@ -28,7 +28,8 @@
"php": "^7.3 || ~8.0.0",
"container-interop/container-interop": "^1.1",
"laminas/laminas-stdlib": "^3.3",
"laminas/laminas-zendframework-bridge": "^1.0"
"laminas/laminas-zendframework-bridge": "^1.0",
"symfony/polyfill-mbstring": "^1.20"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO to be declared should be ext-mbstring, while we can decide to use symfony/polyfill-mbstring in require-dev

@gsteel gsteel deleted the branch laminas:2.14.x July 31, 2024 13:18
@gsteel gsteel closed this Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants